[OCTRL-1091] Environment controller changes for ECS#828
Conversation
…atiblity - matching node names in cluster from partial names yaml templates - add taskID annotation to be able to filter tasks - OCC_CONTROL_PORT overwrites Spec.Control.Port of task
knopers8
changed the title
knopers8
left a comment
knopers8
left a comment
There was a problem hiding this comment.
indeed understanding the intention of each change was difficult given the PR does many things, so i might have skipped something
| securityContext: | ||
| fsGroup: 1100 | ||
| supplementalGroups: [10, 1105] |
There was a problem hiding this comment.
it looks like this does not correspond to any of the three changes?
- matching node names in cluster from partial names yaml templates
- add taskID annotation to be able to filter tasks
- OCC_CONTROL_PORT overwrites Spec.Control.Port of task
can you extract to a separate PR or amend the commit?
| runAsUser: 1100 | ||
| runAsGroup: 1100 |
| // resolveNodeName resolves a short node name (e.g. "flp001") to the full name known to | ||
| // the cluster (e.g. "flp001.cern.ch") by checking whether any dot-separated component | ||
| // of a node's name matches exactly. Returns an error if zero or more than one node matches. | ||
| func resolveNodeName(nodes []v1.Node, nodename string) (string, error) { |
There was a problem hiding this comment.
Please use "nodeName" or "nodename" consistently, i.e. "name" either always starting upper or lower case. IMO "nodeName" is better, since it's two words, but I leave the final decision to you.
| name: readout | ||
| namespace: alice-tasks | ||
| spec: | ||
| nodeName: mtichak-ost.cern.ch |
There was a problem hiding this comment.
I am a bit lost why ".cern.ch" is needed here, but should be absent in the environment templates.
| task.Spec.Arguments = make(map[string]string) | ||
| } | ||
|
|
||
| // TODO: check for containers! |
| case 1: | ||
| return matches[0], nil | ||
| default: | ||
| return "", fmt.Errorf("node name %q is ambiguous, matched: %v", nodename, matches) |
There was a problem hiding this comment.
i appreciate being paranoic!
2 participants
environment controller changes for ECS bridge compatiblity